Skip to content

fix: phone-only accounts could not enrol TOTP or be deleted - #753

Merged
lakhansamani merged 4 commits into
mainfrom
fix/phone-only-account-parity
Aug 7, 2026
Merged

fix: phone-only accounts could not enrol TOTP or be deleted#753
lakhansamani merged 4 commits into
mainfrom
fix/phone-only-account-parity

Conversation

@lakhansamani

@lakhansamani lakhansamani commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Two bugs found in local testing, with the same root cause: paths that treat
email as if every account has one. A mobile signup never gets an email, so
anything keyed on it is unreachable for those users.

1. TOTP enrolment failed — AccountName must be set

Reported symptom: signing up with a mobile number produced
AccountName must be set.

That is pquerna/otp rejecting an empty AccountName — the label the
authenticator app shows next to the code. Enrolment passed user.Email
verbatim, which is empty for a phone-only account.

MFA is on by default, so this is the first thing a mobile signup hits after
verifying
: the account could be created but never finish MFA setup.

Falls back to the phone number, then the user id. The id is a poor label but it
is never empty, so enrolment cannot fail on a missing identifier.

2. A phone-only account could not be deleted at all

DeleteUserRequest.email was required, and it is the only identifier the
endpoint accepted. A phone-only account has no email, so there was no second way
in — the account was permanent.

Now id only, across GraphQL, gRPC and REST.

Breaking change (admin API, agreed in review)

  • GraphQL DeleteUserRequest is now { id: String! }email removed.
  • gRPC DeleteUserRequest takes string id = 2 with min_len = 1.
  • REST POST /v1/admin/delete_user takes {"id": "..."}.
  • The dashboard deletes by id.

Field 1 is reserved, not reused. email was a string and id is a string,
so reusing tag 1 would let an old client's email decode silently as an id —
wire-compatible, semantically wrong, and pointed at a delete. Reserving makes an
old client fail loudly instead:

message DeleteUserRequest {
  reserved 1;
  reserved "email";
  string id = 2 [(buf.validate.field).string.min_len = 1];
}

Scoped to _delete_user / AuthorizerAdminService.DeleteUser. No public-API
surface changes.

⚠️ Needs the proto-breaking-approved label

buf breaking correctly reports:

Previously present field "1" with name "email" on message "DeleteUserRequest" was deleted.

That is the intended change. The repo already has the right lever for this — a
maintainer applies proto-breaking-approved and the gate becomes
continue-on-error for this PR only. I deliberately did not add a
buf.yaml ignore: that would suppress the check for the whole admin file and
hide the next accidental break.

buf lint passes, and make proto-check confirms gen/ is not stale.

Tests

  • TOTP enrolment across all three identifier shapes — phone-only (the reported
    case), email-only, and neither (falls back to the id). Verified fail-before:
    reverting the fix reproduces AccountName must be set on exactly the
    phone-only and neither cases.
  • Deleting a phone-only account by id — the case that was previously impossible.
  • An empty id is rejected at the service (GraphQL) and by min_len (gRPC);
    without that guard "" falls through to a lookup whose result depends on the
    storage backend rather than on intent.
  • Existing delete tests migrated to id across GraphQL, gRPC and REST.

Verified

make lint (0 issues), go build, go vet, full Go suite, dashboard
typecheck + build — all green. make generate-graphql and make proto-gen
re-run and gen/ committed.

Note on scope

These are pre-existing bugs, not regressions from the audit work — but they were
surfaced by testing it, and both sit in the same email-centric assumption. Worth
a sweep for other places that key on email where an account may not have one;
this PR fixes the two that were actually hit.

@lakhansamani
lakhansamani force-pushed the fix/phone-only-account-parity branch from 52cb1c9 to c5afc04 Compare August 7, 2026 09:47
Two bugs from local testing, same root cause: paths that treat email as if
every account has one. A mobile signup never gets an email, so anything keyed
on it is unreachable for those users.

TOTP enrolment failed outright with pquerna/otp's "AccountName must be set".
AccountName is the label the authenticator app shows and the enrolment passed
user.Email verbatim, which is empty for a phone-only account; the library
rejects an empty value. MFA is on by default, so this is the first thing a
mobile signup hits after verifying - the account could be created but never
finish setup. Falls back to the phone number, then the user id, so enrolment
cannot fail on a missing identifier.

Admin delete accepted ONLY an email (DeleteUserRequest.email was required), so
a phone-only account could not be deleted at all - there was no second way in
and the account was permanent. Now id-or-email with id preferred, mirroring the
shape GetUserRequest already used. Email is kept for existing callers; the
handler rejects a request carrying neither, since "exactly one of" is not
expressible in the schema or in protobuf.

BREAKING (admin API, agreed): DeleteUserRequest.email is no longer required.
Callers sending email are unaffected. gRPC gains DeleteUserRequest.id = 2 and
drops the min_len constraint on email. The dashboard now deletes by id.

Tests cover TOTP enrolment across all three identifier shapes (phone-only,
email-only, neither), deleting a phone-only account by id, email still working,
and the neither-supplied rejection.
@lakhansamani
lakhansamani force-pushed the fix/phone-only-account-parity branch from c5afc04 to 5bfc821 Compare August 7, 2026 10:14
@lakhansamani lakhansamani added the proto-breaking-approved Maintainer-approved intentional proto breaking change; relaxes buf breaking gate label Aug 7, 2026
Found reviewing this PR before merge, and the more serious of the two bugs here.

DeleteUser's post-delete cleanup looks up rows to remove by email and by phone
number. generateAndStoreOTP writes both as plain strings, so an account holding
only one of them stores the OTHER as "" - not NULL. The lookups are
`WHERE email = ?` / `WHERE phone_number = ?`, so passing "" MATCHES every other
account in the same shape, and the row it returns is then deleted.

Deleting an email-only account therefore ran GetOTPByPhoneNumber(ctx, "") and
could delete a DIFFERENT email-only account's live OTP - logging that user out
mid-verification with nothing they could observe to explain it. That half is
reachable today. The email half becomes reachable now that phone-only accounts
can be deleted at all, which is what this PR enables.

Guarded at the call site rather than in each storage provider: the empty value
is meaningless here, and six backends would each have to get the same guard
right for it to hold.

The regression test is deliberately deterministic - the deleted account has no
OTP of its own, so the empty-phone lookup can only match the bystander's. An
earlier version had both accounts holding OTPs and passed without the fix,
because the lookup happened to return the deleted account's own row; a test
that only sometimes reproduces is not a test.
proto-check-clients diffs gen/ts against a fresh generate; the DeleteUser
field change landed in proto/ without it.
make treats a bare # as a comment mid-assignment, so the value collapsed to
`.git`. Without subdir=proto buf read the repo root as the module root and
every import failed to resolve; without branch=origin/main it compared the
working tree against itself. The target reported 40+ bogus errors and never
reached a real comparison, so no local run of it could catch a breaking change.
@lakhansamani
lakhansamani merged commit 0a401cb into main Aug 7, 2026
6 checks passed
lakhansamani added a commit to authorizerdev/authorizer-proto-go that referenced this pull request Aug 7, 2026
Regenerated from BSR after authorizerdev/authorizer#753.

BREAKING CHANGE: DeleteUserRequest.Email and GetEmail() are removed; use Id
and GetId(). Requires server 2.4.0+. Proto field 1 is reserved rather than
reused, so an old client's email cannot silently decode as an id on a delete.
lakhansamani added a commit to authorizerdev/authorizer-proto-python that referenced this pull request Aug 7, 2026
Regenerated from BSR after authorizerdev/authorizer#753.

BREAKING CHANGE: DeleteUserRequest.email is removed; use id. Requires server
2.4.0+.
lakhansamani added a commit to authorizerdev/authorizer-js that referenced this pull request Aug 7, 2026
Mirrors authorizerdev/authorizer#753.

BREAKING CHANGE: DeleteUserRequest.email is replaced by DeleteUserRequest.id.
Requires server 2.4.0+.
lakhansamani added a commit to authorizerdev/authorizer-py that referenced this pull request Aug 7, 2026
Mirrors authorizerdev/authorizer#753.

BREAKING CHANGE: DeleteUserRequest.email is replaced by DeleteUserRequest.id.
Requires server 2.4.0-rc.16+ and authorizer-proto 0.2.0rc1+.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

proto-breaking-approved Maintainer-approved intentional proto breaking change; relaxes buf breaking gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant